Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AdminSiteOTPRequiredMixin modified #370

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

aseem-hegshetye
Copy link
Contributor

When admin user is staff user and is active, but doesnt have OTP setup, AdminSiteOTPRequiredMixin will redirect user to OTP setup page after login.

When admin user is staff user and is active, but doesnt have OTP setup, AdminSiteOTPRequiredMixin will redirect user to OTP setup page after login.
Copy link
Collaborator

@moggers87 moggers87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are required and imports need to be ordered (see failing Travis build)

@@ -44,21 +43,30 @@ def test_default_admin(self):

@override_settings(ROOT_URLCONF='tests.urls_otp_admin')
class OTPAdminSiteTest(UserMixin, TestCase):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I test this class alone, all tests pass.
When I run entire test suit, 2 tests in this class fail

  1. test_otp_admin_without_otp
  2. test_otp_admin_without_otp_named_url

When running entire test suite, self.client.get('/otp_admin/', follow=True) returns login page url /account/login/?next=%2Fotp_admin%2F, when its supposed to return a OTP setup url /account/two_factor/setup/.
I was unable to find why this happens.
Any help is appreciated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I just test this class: AdminSiteOTPRequiredMixin().login() func executes.
When I run entire test suite - AdminSiteOTPRequiredMixin().login() func never executes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've not had a chance to look at this yet and I'm not overly familiar with the admin code.

I would imagine that our tests aren't quite as isolated from eachother as we'd actually want. Have you tried running just the test_admin module and then the class those two tests belong to? That might help narrow down where the problem is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept AdminSiteTest and OTPAdminSiteTest in test_admin.py and tried to test make test TARGET=tests.test_admin
They work fine as long as there is no get request made in AdminSiteTest class.
If I make that get request in AdminSiteTest.test_default_admin I get error in OTPAdminSiteTest class.

class AdminSiteTest(UserMixin, TestCase):

    def setUp(self):
        super().setUp()
        self.user = self.create_superuser()
        self.login_user()
        
    def test_default_admin(self):
        with self.settings(ROOT_URLCONF='tests.urls_admin'):
            # if I comment the line below, test_admin.py tests successfully.
            response = self.client.get('/admin/')


class OTPAdminSiteTest(UserMixin, TestCase):
    def setUp(self):
        super().setUp()
        self.user = self.create_superuser()
        self.login_user()

    def test_otp_admin_without_otp(self):
        with self.settings(ROOT_URLCONF='tests.urls_otp_admin'):
            response2 = self.client.get('/otp_admin/', follow=True)
            redirect_to = reverse('two_factor:setup')
            self.assertRedirects(response2, redirect_to)

Error:
Response redirected to '/admin/', expected '/account/two_factor/setup/'

So in conclusion, error in OTPAdminSiteTest class depends on which test was run previously. If I were to substitute AdminSiteTest with AdminPatchTest in the above code, my error will change to following:
Response redirected to '/account/login/?next=/otp_admin/', expected '/account/two_factor/setup/

redirect_to = request.POST.get(REDIRECT_FIELD_NAME, request.GET.get(REDIRECT_FIELD_NAME))

# if user (is_active and is_staff)
if request.method == "GET" and AdminSite().has_permission(request):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not self.has_permission(request)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.has_permission() func checks if user (is_staff, is_active and is_verified) .
AdminSite().has_permission() checks if user (is_staff, is_active ). Then based on if user is not verified (OTP not setup) we want to redirect to OTP setup page.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super() should be used instead then. This is a mixin and we don't know what other checks users might have/want so there might be other classes involved, not just AdminSite.

@aseem-hegshetye
Copy link
Contributor Author

aseem-hegshetye commented Aug 12, 2020

Can anyone help please?

@Jenselme
Copy link

@aseem-hegshetye @moggers87 any updates on this? Can I help? I'd like this feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants